[SROA] Prevent load atomic vector from being generated #112432

jofrn · 2024-10-15T20:15:10Z

load atomic <n x T> is illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

These are illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

llvmbot · 2024-10-15T20:15:48Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: None (jofrn)

Changes

These are illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

Full diff: https://github.com/llvm/llvm-project/pull/112432.diff

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/SROA.cpp (+5)
(added) llvm/test/Transforms/SROA/atomic-vector.ll (+19)

diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 92589ab17da313..450ecdf20ef009 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -2853,6 +2853,11 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {
 
   bool visitLoadInst(LoadInst &LI) {
     LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
+
+    // load atomic vector would be generated, which is illegal
+    if (LI.isAtomic() && NewAI.getAllocatedType()->isVectorTy())
+      return false;
+
     Value *OldOp = LI.getOperand(0);
     assert(OldOp == OldPtr);
 
diff --git a/llvm/test/Transforms/SROA/atomic-vector.ll b/llvm/test/Transforms/SROA/atomic-vector.ll
new file mode 100644
index 00000000000000..d43ae653fba1dd
--- /dev/null
+++ b/llvm/test/Transforms/SROA/atomic-vector.ll
@@ -0,0 +1,19 @@
+; RUN: opt < %s -passes='sroa' -S 2>&1 | FileCheck %s --check-prefix=ERR
+; RUN: opt < %s -passes='sroa' -S | FileCheck %s
+
+define float @atomic_vector() {
+; ERR-NOT: atomic load operand must have integer, pointer, or floating point type!
+; ERR-NOT:   <1 x float>  {{%.*}} = load atomic volatile <1 x float>, ptr {{%.*}} acquire, align 4
+; CHECK:      %1 = alloca <1 x float>, align 4
+; CHECK-NEXT: store <1 x float> undef, ptr %1, align 4
+; CHECK-NEXT: %2 = load atomic volatile float, ptr %1 acquire, align 4
+; CHECK-NEXT: ret float %2
+  %1 = alloca <1 x float>
+  %2 = alloca <1 x float>
+  %3 = alloca ptr
+  call void @llvm.memcpy.p0.p0.i64(ptr %2, ptr %1, i64 4, i1 false)
+  store ptr %2, ptr %3
+  %4 = load ptr, ptr %3
+  %5 = load atomic volatile float, ptr %4 acquire, align 4
+  ret float %5
+}

jofrn · 2024-10-15T20:17:14Z

This is a follow-up to #111414, which used -disable-verify. This commit prevents atomic load vector at all.

shiltian · 2024-10-15T20:23:35Z

llvm/lib/Transforms/Scalar/SROA.cpp

@@ -2853,6 +2853,11 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {

  bool visitLoadInst(LoadInst &LI) {
    LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
+
+    // load atomic vector would be generated, which is illegal
+    if (LI.isAtomic() && NewAI.getAllocatedType()->isVectorTy())


Is it illegal from LangRef's perspective?

It is at least illegal from the perspective of IR/Verifier.

It looks like this is written here.

The restriction is dumb and we should relax it. Instead of just hardcoding isVectorTy here, should have some kind of LoadInst::isValidAtomicType or something

The verifier check should be moved into a LoadInst helper

No alloca reference, just the type. The implementation should match Verifier::visitLoadInst, the isIntOrIntPtrTy || isFloatingPointTy and checkAtomicMemAccessSize (also add some non-atomic size tests?)

We need to ensure an atomic does not have a vector type. If the load is atomic and the alloca that will lend its type over has a vector type, then we will generate an atomic vector, which are illegal. We need to ensure that this doesn't occur.

No, you need to ensure the atomic is a valid type for an atomic load.

Alternatively you can do the load with the equivalent sized type and then bitcast (which is why this restriction is dumb in the first place, the lowering can always do the same)

We are ensuring it is a valid type in the case under question, by checking the alloca's type. The invalid load will not be generated if the alloca has a vector type as that type will overwrite the load's type.

We don't want any loads that are atomic to have a vector type, regardless of whether the atomic itself is already valid; so checking if the atomic is more valid before translating it in AllocaSliceRewriter will miss cases where we form an invalid atomic during SROA (and then later form a vector type with it in visitLoadInst). Even though it is not likely as SROA probably won't form these, it illustrates why we don't need the extra checks here.

llvm/test/Transforms/SROA/atomic-vector.ll

arsenm · 2024-10-16T04:11:57Z

llvm/test/Transforms/SROA/atomic-vector.ll

+; CHECK-NEXT:    [[TMP2:%.*]] = load atomic volatile float, ptr [[TMP1]] acquire, align 4
+; CHECK-NEXT:    ret float [[TMP2]]
+;
+  %1 = alloca <1 x float>


Use named values in tests

arsenm · 2024-10-16T04:12:05Z

llvm/test/Transforms/SROA/atomic-vector.ll

+; CHECK-NEXT:    ret float [[TMP2]]
+;
+  %1 = alloca <1 x float>
+  %2 = alloca <1 x float>


Test an integer and pointer vector too

jofrn · 2024-10-18T06:41:13Z

llvm/test/Transforms/SROA/basictest.ll

-; CHECK-NEXT:    ret i32 [[A_0_LOAD_EXT]]
+; CHECK-NEXT:    [[A:%.*]] = alloca i1, align 1
+; CHECK-NEXT:    [[V:%.*]] = load atomic i32, ptr [[A]] seq_cst, align 4
+; CHECK-NEXT:    ret i32 [[V]]


Due to this test, we may want to check only the type without DL. Adding too many checks hinders optimization.

jofrn · 2024-10-18T06:43:14Z

llvm/lib/IR/Instructions.cpp

+  if (AO == AtomicOrdering::Release || AO == AtomicOrdering::AcquireRelease)
+    return false;
+  unsigned Size = DL.getTypeSizeInBits(Ty);
+  return Size >= 8 && !(Size & (Size - 1));


basictest.ll fails due to checking DL here.

arsenm · 2024-10-20T14:47:04Z

llvm/lib/IR/Instructions.cpp

+  if (!Ty->isIntOrPtrTy() && !Ty->isFloatingPointTy())
+    return false;
+  return true;


Direct return of boolean expression

arsenm · 2024-10-20T14:47:29Z

llvm/lib/Transforms/Scalar/SROA.cpp

@@ -2853,6 +2853,11 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {

  bool visitLoadInst(LoadInst &LI) {
    LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
+
+    // load atomic vector would be generated, which is illegal


Suggested change

// load atomic vector would be generated, which is illegal

// Load atomic vector would be generated, which is illegal.

arsenm · 2024-10-20T14:47:56Z

llvm/test/Transforms/SROA/atomic-vector.ll

+  %indirect = load ptr, ptr %direct
+  %ret = load atomic volatile ptr, ptr %indirect acquire, align 4
+  ret ptr %ret
+}


Test a <2 x i16> or some other real vector. 1 x is a degenerate case

arsenm · 2024-10-20T22:28:27Z

llvm/include/llvm/IR/Instructions.h

@@ -252,7 +252,8 @@ class LoadInst : public UnaryInstruction {

  /// Returns false if this type would be invalid in the
  /// creation of a load atomic instruction.
-  static bool isValidAtomicTy(Type *Ty);
+  static bool isValidAtomicTy(Type *Ty, const DataLayout *DL = nullptr,


DataLayout must be mandatory

Then we need to omit it entirely because otherwise we will be checking the type size of the alloca, which we want to avoid if to optimize basictest.ll.

arsenm · 2024-10-20T22:28:31Z

llvm/lib/IR/Instructions.cpp

  if (!Ty->isIntOrPtrTy() && !Ty->isFloatingPointTy())
    return false;
+  if (AO == AtomicOrdering::Release || AO == AtomicOrdering::AcquireRelease)
+    return false;
+  if (DL) {


arsenm · 2024-10-20T22:30:25Z

llvm/test/Transforms/SROA/atomic-vector.ll

+  %indirect = load ptr, ptr %direct
+  %ret = load atomic volatile i32, ptr %indirect acquire, align 4
+  ret i32 %ret
+}


Add test for the non-byte illegal case?

[SROA] Prevent load atomic vector from being generated

cb05b56

These are illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

jofrn requested review from arsenm, shiltian and Pierre-vh October 15, 2024 20:15

llvmbot added the llvm:transforms label Oct 15, 2024

shiltian reviewed Oct 15, 2024

View reviewed changes

llvm/test/Transforms/SROA/atomic-vector.ll Outdated Show resolved Hide resolved

Autogenerate assertions

0c61505

jofrn force-pushed the atomicvec-sroa branch from 77b4341 to 0c61505 Compare October 15, 2024 21:18

arsenm reviewed Oct 16, 2024

View reviewed changes

Add i32,ptr tests and rename variables

083346d

jofrn force-pushed the atomicvec-sroa branch from 341ed5a to 4685582 Compare October 17, 2024 04:10

memcpy size must be >=8 to generate load atomic <1 x ptr>

cf47505

jofrn force-pushed the atomicvec-sroa branch from 4685582 to cf47505 Compare October 17, 2024 08:10

Use isValidAtomicTy

ef4fe92

llvmbot added the llvm:ir label Oct 18, 2024

jofrn added 2 commits October 17, 2024 23:48

Add AtomicOrdering to isValidAtomicTy interface

bdebaa8

Updated basictest.ll for checking getTypeSizeInBits

d68d923

jofrn commented Oct 18, 2024

View reviewed changes

Revert basictest.ll changes and DL check

1c1aae7

jofrn requested review from arsenm and shiltian October 18, 2024 16:44

arsenm reviewed Oct 20, 2024

View reviewed changes

Add <2 x i32> test. Add DL as optional to isValidAtomicTy

67e0560

arsenm reviewed Oct 20, 2024

View reviewed changes

jofrn force-pushed the atomicvec-sroa branch 2 times, most recently from 198a549 to db0a8fe Compare October 21, 2024 11:01

Added nonbyte test. Removed interface to isValidAtomicTy.

e2e58ee

jofrn force-pushed the atomicvec-sroa branch from db0a8fe to e2e58ee Compare October 21, 2024 11:04

jofrn requested a review from arsenm October 22, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SROA] Prevent load atomic vector from being generated #112432

[SROA] Prevent load atomic vector from being generated #112432

jofrn commented Oct 15, 2024 •

edited

Loading

llvmbot commented Oct 15, 2024 •

edited

Loading

jofrn commented Oct 15, 2024

shiltian Oct 15, 2024

jofrn Oct 15, 2024

jofrn Oct 15, 2024 •

edited

Loading

arsenm Oct 16, 2024

arsenm Oct 16, 2024

arsenm Oct 16, 2024

jofrn Oct 16, 2024 •

edited

Loading

arsenm Oct 16, 2024

jofrn Oct 16, 2024 •

edited

Loading

jofrn Oct 17, 2024 •

edited

Loading

arsenm Oct 16, 2024

arsenm Oct 16, 2024

jofrn Oct 18, 2024

jofrn Oct 18, 2024

arsenm Oct 20, 2024

arsenm Oct 20, 2024

arsenm Oct 20, 2024

arsenm Oct 20, 2024

jofrn Oct 21, 2024

arsenm Oct 20, 2024

arsenm Oct 20, 2024

	// load atomic vector would be generated, which is illegal
	// Load atomic vector would be generated, which is illegal.

[SROA] Prevent load atomic vector from being generated #112432

Are you sure you want to change the base?

[SROA] Prevent load atomic vector from being generated #112432

Conversation

jofrn commented Oct 15, 2024 • edited Loading

llvmbot commented Oct 15, 2024 • edited Loading

jofrn commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

jofrn Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn commented Oct 15, 2024 •

edited

Loading

llvmbot commented Oct 15, 2024 •

edited

Loading

jofrn Oct 15, 2024 •

edited

Loading

jofrn Oct 16, 2024 •

edited

Loading

jofrn Oct 16, 2024 •

edited

Loading

jofrn Oct 17, 2024 •

edited

Loading